Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clientv3/concurrency.Mutex.Lock() - preserve invariant #10153

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

funny-falcon
Copy link
Contributor

Convenient invariant:

  • if werr == nil then lock is supposed to be locked at the moment.

While we could not be confident in stronger invariant ('is exactly locked'),
it were inconvenient that previous code could return werr == nil when
it released Lock.

It could happen when ctx is canceled/timeouted exactly after waitDeletes
successfully returned werr == nil and before <-ctx.Done() checked.
While such situation is very rare, it is still possible.

fixes #10111

@codecov-io
Copy link

codecov-io commented Oct 5, 2018

Codecov Report

Merging #10153 into master will increase coverage by 0.16%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10153      +/-   ##
==========================================
+ Coverage   71.54%   71.71%   +0.16%     
==========================================
  Files         390      390              
  Lines       36307    36306       -1     
==========================================
+ Hits        25975    26036      +61     
+ Misses       8530     8461      -69     
- Partials     1802     1809       +7
Impacted Files Coverage Δ
clientv3/concurrency/mutex.go 59.57% <0%> (-2.93%) ⬇️
pkg/logutil/zap_grpc.go 47.61% <0%> (-4.77%) ⬇️
etcdserver/api/v2http/client.go 85.51% <0%> (-1.21%) ⬇️
clientv3/retry_interceptor.go 65.78% <0%> (-1.06%) ⬇️
pkg/proxy/server.go 60.2% <0%> (-0.68%) ⬇️
etcdserver/api/rafthttp/stream.go 79.39% <0%> (-0.43%) ⬇️
clientv3/watch.go 91.73% <0%> (-0.43%) ⬇️
mvcc/watchable_store.go 84.21% <0%> (-0.36%) ⬇️
clientv3/balancer/grpc1.7-health.go 59.01% <0%> (-0.3%) ⬇️
etcdserver/server.go 74.08% <0%> (+0.5%) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6976819...64e8b2e. Read the comment docs.

Convenient invariant:
- if werr == nil then lock is supposed to be locked at the moment.

While we could not be confident in stronger invariant ('is exactly locked'),
it were inconvenient that previous code could return `werr == nil` after
Mutex.Unlock.

It could happen when ctx is canceled/timeouted exactly after waitDeletes
successfully returned werr == nil and before `<-ctx.Done()` checked.
While such situation is very rare, it is still possible.

fixes etcd-io#10111
@funny-falcon funny-falcon force-pushed the fix-client-mutex-lock-10111 branch from 1d624bb to 64e8b2e Compare October 5, 2018 11:23
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, defer to @xiang90

@jingyih
Copy link
Contributor

jingyih commented Oct 5, 2018

lgtm. Thanks for fixing.

@xiang90
Copy link
Contributor

xiang90 commented Oct 8, 2018

lgtm

@xiang90 xiang90 merged commit b046a37 into etcd-io:master Oct 8, 2018
@jingyih
Copy link
Contributor

jingyih commented Oct 8, 2018

@gyuho @xiang90 I can backport this fix to release 3.3, 3.2 and 3.1, if you haven't already started doing so. Please let me know.

@gyuho
Copy link
Contributor

gyuho commented Oct 8, 2018

@jingyih Sounds good. Could you also update the changelogs as we backport? Thanks!

jingyih added a commit to jingyih/etcd that referenced this pull request Oct 9, 2018
jingyih added a commit that referenced this pull request Oct 9, 2018
…3-origin-release-3.3

clientv3: automated cherry pick of #10153 to release-3.3
jingyih added a commit that referenced this pull request Oct 9, 2018
…3-origin-release-3.2

clientv3: automated cherry pick of #10153 to release-3.2
jingyih added a commit that referenced this pull request Oct 9, 2018
…3-origin-release-3.1

clientv3: automated cherry pick of #10153 to release 3.1
jingyih added a commit that referenced this pull request Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

clientv3/concurrency: return error in Mutex.Lock if lock were unlocked due to closed context
5 participants